Skip to content

build(windows): Do not try to build minizip before zlib.#1965

Closed
MrSoup678 wants to merge 6 commits into
AcademySoftwareFoundation:mainfrom
MrSoup678:main
Closed

build(windows): Do not try to build minizip before zlib.#1965
MrSoup678 wants to merge 6 commits into
AcademySoftwareFoundation:mainfrom
MrSoup678:main

Conversation

@MrSoup678
Copy link
Copy Markdown
Contributor

No description provided.

@MrSoup678 MrSoup678 force-pushed the main branch 3 times, most recently from 8d741d8 to 908dd19 Compare April 12, 2024 10:42
Signed-off-by: Sławomir Śpiewak <sosw.slawomir.spiewak@gmail.com>
@doug-walker
Copy link
Copy Markdown
Collaborator

@SlawekNowy , thanks for the PR. Would you please provide more detail about what scenarios require this fix? In other words, most Windows builds succeed already without this, so it must be something more specific about the way you are building.

@MrSoup678
Copy link
Copy Markdown
Contributor Author

This is the local windows build. Fetched the repository and built it using Visual Studio generator. Both minizip and zlib are managed by opencolorio. Visual studio 2022 Community x64.

@MrSoup678
Copy link
Copy Markdown
Contributor Author

Oh. I forgot to mention I run --parallel at cmake build step.

Also it seems that did not fix my problems?

@MrSoup678 MrSoup678 marked this pull request as draft April 18, 2024 14:29
@MrSoup678 MrSoup678 marked this pull request as ready for review April 29, 2024 13:49
@MrSoup678
Copy link
Copy Markdown
Contributor Author

The build is done using cmake .. from build directory (from ocio dir), and then building generated project.

@MrSoup678
Copy link
Copy Markdown
Contributor Author

I'll admit this is not standard setup, but this repo's CMakeLists does fetch deps...
And besides that is standard setup on Linux...

@MrSoup678 MrSoup678 marked this pull request as draft May 7, 2024 13:55
@MrSoup678
Copy link
Copy Markdown
Contributor Author

From parent project's discord:

ok cmake. You get a race condition without the deps' root set, but you don't if I do set the root directories of deps? what?

@stephen-yee-adsk
Copy link
Copy Markdown

Hi Slawek,

I encountered the same problem on Windows. It seems that minizip-ng sometimes builds before zlib, which causes a build error, because minizip-ng depends on zlib. I was able to fix it locally by adding a dependency on Zlib in the ExternalProject_add call in Installminizip-ng.cmake.

i.e.

    ExternalProject_Add(minizip-ng_install
        GIT_REPOSITORY "https://github.com/zlib-ng/minizip-ng.git"
        GIT_TAG "${minizip-ng_VERSION}"
        GIT_CONFIG advice.detachedHead=false
        GIT_SHALLOW TRUE
        PREFIX "${_EXT_BUILD_ROOT}/libminizip-ng"
        BUILD_BYPRODUCTS ${minizip-ng_LIBRARY}
        CMAKE_ARGS ${minizip-ng_CMAKE_ARGS}
        EXCLUDE_FROM_ALL TRUE
        BUILD_COMMAND ""
        INSTALL_COMMAND
            ${CMAKE_COMMAND} --build .
                            --config ${CMAKE_BUILD_TYPE}
                            --target install
                            --parallel
        DEPENDS ZLIB::ZLIB        # minizip-ng depends on zlib
    )

Could you try this and see if it fixes your build?

@MrSoup678
Copy link
Copy Markdown
Contributor Author

@stephen-yee-adsk Confirmed fixed, by project head.

Fix an occasional race condition caused by zlib building before minizip-ng.
@MrSoup678 MrSoup678 marked this pull request as ready for review June 4, 2024 12:53
Comment thread share/cmake/modules/FindExtPackages.cmake Outdated
@MrSoup678 MrSoup678 requested a review from doug-walker June 12, 2024 15:23
@doug-walker
Copy link
Copy Markdown
Collaborator

@SlawekNowy , thanks for the update, the code looks perfect now, but unfortunately it looks like you did not sign your latest commit? The DCO sign-off check needs to pass before we're able to merge it. Usually "git commit --amend -s" and then re-pushing will fix it.

This reverts commit 025e7c0.

Signed-off-by: MrSoup678 <sosw.slawomir.spiewak@gmail.com>
@MrSoup678 MrSoup678 force-pushed the main branch 3 times, most recently from 2758870 to 6fb350b Compare June 14, 2024 18:05
@MrSoup678
Copy link
Copy Markdown
Contributor Author

Closing in favor of #1986.

@MrSoup678 MrSoup678 closed this Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants